Add parameter default_planning_scene to load planning scene geometry on startup
#2949
Conversation
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
| std::string path = nh->get_parameter("default_planning_scene").as_string(); | ||
| std::fstream file_stream; | ||
| file_stream.open(path, std::fstream::in); | ||
| if (!file_stream.is_open() || !ps->loadGeometryFromStream(file_stream)) |
There was a problem hiding this comment.
I think this is too dangerous because you're not locking the planning scene when writing to it (see
You can get thread safe access like this:
{
planning_scene_monitor::LockedPlanningSceneRW ps = planning_display_->getPlanningSceneRW();
if (ps)
FANCY MODIFICATIONS
} // End scope to release lockSigned-off-by: Paul Gesel <paul.gesel@picknik.ai>
|
This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete. |
github-actions
bot
added
the
stale
|
@pac48 Should we close this? |
github-actions
bot
removed
the
stale
|
@sjahr We should try to merge this. I think the change to the test should either be deleted or more effort has to be put in to make it not flaky. |
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
pac48
force-pushed
the
pr-load-default-planning-scene
branch
from
2355089
to
4eef9ee
Compare
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
Signed-off-by: Paul Gesel <paul.gesel@picknik.ai>
There was a problem hiding this comment.
I think it is a useful addition but it could be hardened a bit to make it harder to misuse. Also can you add the PR description as a documentation comment on top of this function call? At the moment there is nothing explaining what this is doing or how it is supposed to be used
| @@ -290,8 +290,26 @@ int main(int argc, char** argv) | |||
| const auto moveit_cpp = std::make_shared<moveit_cpp::MoveItCpp>(nh, moveit_cpp_options); | |||
| const auto planning_scene_monitor = moveit_cpp->getPlanningSceneMonitorNonConst(); | |||
|
|
|||
| if (planning_scene_monitor->getPlanningScene()) | |||
| if (auto ps = planning_scene_monitor->getPlanningScene()) | |||
There was a problem hiding this comment.
Given that ps is never used, would make sense to revert this change?
| { | ||
| if (nh->has_parameter("default_planning_scene")) |
There was a problem hiding this comment.
I am a bit concerned about tying this to only the existence of the parameter. How about checking whether or not the parameter is set.
const auto default_planning_scene_file = std::string();
nh->get_parameter_or("default_planning_scene", default_planning_scene_file, std::string());
// Maybe add function to validate the string pattern if it actually representing path
if(validateFilePath(default_planning_scene_file))
{
...
}
Description
This PR adds a new parameter to the
move_groupnode that allows users to specify a planning scene file to load on startup. The parameter is a string containing the absolute file path. The parameter can be set with either$(find-pkg-share pkg-wth-planning-file)for XML orament_index_python.packages.get_package_share_directoryin Python to facilitate specifying the path. If the parameter is not specified, themove_groupnode will behave as it previously did. If there is an error loading the file, the node will exit with a fatal error.Checklist